Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: memoize field row containers #6189

Merged
merged 3 commits into from
Apr 25, 2023
Merged

Conversation

foochifa
Copy link
Contributor

@foochifa foochifa commented Apr 25, 2023

Problem

Currently we have a lot of unnecessary re-renders on our Build and Design Content. Many components are re-rendering despite no changes to their properties. This is caused by the large number of hooks we use in the component.

This causes extreme lag in large forms as the re-renders linearly increments the page rendering time for every single action (dragging, selecting, modifying fields in the admin-form view)

This is especially detrimental for dragging fields, as it can cause the admin view for large forms to be almost impossible to navigate.

Closes #6121

Solution

Move out some of the hooks from FieldRowContainer to its parent BuilderField which are causing most of the unnecessary re-renders

Use react memo to memoize the FieldRowContainer with deep comparison of props.

Breaking Changes

  • No - this PR is backwards compatible

Before & After Screenshots

BEFORE:

Large amount of re-render time as seen in the right.
image

Screen.Recording.2023-04-25.at.1.16.24.PM.mov

AFTER:

image

Screen.Recording.2023-04-25.at.1.15.14.PM.mov

Tests

To ensure the feature works
Use react dev tool -> profiler

  • go to a form in your admin view
  • start profiler recording
  • shift some of the fields around
  • stop recording, you should see that most fields are not re-rendered at each step, and they are memoized at the FieldRow level

Go to a large form

  • try to drag a field. The dragging of the field (before dropping) should feel smooth, and there should be minimal jitters.

To ensure core features are still intact

  • In any storage form, ensure the following functionality is still working
    • Create a new field
    • Edit the field properties of any field
    • Edit the field properties, do not save, click away (close the drawer or select another field) the discard editing modal should appear
    • Drag a field to change its position, field id should be updated too
    • Duplicate a field
    • Attempt to delete a field, the modal should open, and can be cancelled
    • Delete the field, modal should open, and field can be deleted
    • Preview the form. Make a submission
    • Open the form to respondents. Make a submission
  • Do the same steps above for an email form
  • Do the same steps on mobile

Notes

There are still a few unnecessary re-renders upon drag and dropping of the fields.

Ideally, only the fields affected (field id changed) should re-render and all other fields should be unaffected. However, currently there is still some existing hooks that causes a re-render for all the fields even if they are memoized

@foochifa foochifa marked this pull request as ready for review April 25, 2023 06:49
@foochifa foochifa requested review from KenLSM and justynoh April 25, 2023 06:53
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@foochifa foochifa merged commit 3848a0f into develop Apr 25, 2023
@foochifa foochifa deleted the perf/memoizing-field-row branch April 25, 2023 07:46
@karrui
Copy link
Contributor

karrui commented Apr 25, 2023

All fields rerender after drag and drop due to recalculation of question number. May be able to reduce impact if we separate the field title from the rest of the field and memo them

This was referenced Apr 26, 2023
@foochifa
Copy link
Contributor Author

Do we have hooks to recalculate the field ids? Probably a bit more investigation needs to be done haha

Cause seems like the hooks are causing the re-renders, as seen below the 6 fields that got their ids updated did get re-rendered correctly. But it seems like some hooks got called that resulted in all the other fields getting re-rendered even though their props didnt change 😅

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix drag lag in form builder
4 participants